-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Change ChainDescriptions into blobs #3787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
/// The epoch in which the chain is created. | ||
pub epoch: Epoch, | ||
/// Blob IDs of the committees corresponding to epochs. | ||
pub committees: BTreeMap<Epoch, Vec<u8>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Vec<u8>
instead of a blob id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this blob depends on other blobs, which complicates chain initialization, so we thought we'd put the full committees in here for now. We could change that as an optimization later.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is inaccurate then, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right! Yes, it is.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
} | ||
} | ||
|
||
/// The configuration for a new chain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "The initial configuration..."
Perhaps InitialChainConfig
is a better name as a result?
|
||
/* TODO: figure out if this can be fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT you can remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(We could take one example chain description and assert its hash, just so we can't inadvertently change how chain IDs are computed. Not sure if that's useful.)
RemoteNode::download_blobs(&blob_ids, validators, self.blob_download_timeout) | ||
.await | ||
.ok_or(LocalNodeError::BlobsNotFound(blob_ids))?; | ||
self.local_node.storage_client().write_blobs(&blobs).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now the remote node could have tricked us, and the chain might not actually have been created.
I think we need to use something like ChainClient::update_local_node_with_blobs_from
, which downloads proof from the validators that the blob was created. Except if we already have at least one correctly signed certificate from this chain—its first block can also count as a proof, I guess (if we double-check the committee information with the admin chain).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good point 🤔
let mut validators_vec = validators.iter().collect::<Vec<_>>(); | ||
validators_vec.shuffle(&mut rand::thread_rng()); | ||
for remote_node in validators_vec { | ||
let info = self.chain_info(chain_id, validators).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, at this point we only need the next block height. Why not just set it to zero if we don't have the chain at all?
let blob = RemoteNode::download_blob(&nodes, chain_desc_id, self.blob_download_timeout) | ||
.await | ||
.ok_or(LocalNodeError::BlobsNotFound(vec![chain_desc_id]))?; | ||
self.local_node.storage_client().write_blob(&blob).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: We can't trust that blob yet.
// our inbox, and include it before all other messages. | ||
rearranged = IncomingBundle::put_openchain_at_front(&mut pending_message_bundles); | ||
ensure!(rearranged, LocalNodeError::InactiveChain(self.chain_id)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nice! Much less messy now! 🎉)
@@ -314,7 +314,7 @@ impl<N: ValidatorNode> RemoteNode<N> { | |||
} | |||
|
|||
#[instrument(level = "trace", skip(validators))] | |||
async fn download_blob( | |||
pub async fn download_blob( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a doc comment that this downloads a blob but doesn't verify that it has actually been published and accepted by a quorum of validators.
ChainError::InvalidBlockTimestamp | ||
); | ||
|
||
self.ensure_is_active(local_time).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do that before ensuring the ensure!
above: If the chain is initialized with a timestamp, we should ensure that the first block's timestamp is no earlier than that.
So maybe the ensure!
and set
should go back into execute_block_inner
?
@@ -508,6 +513,8 @@ impl Block { | |||
self.oracle_blob_ids().contains(blob_id) | |||
|| self.published_blob_ids().contains(blob_id) | |||
|| self.created_blob_ids().contains(blob_id) | |||
|| (blob_id.blob_type == BlobType::ChainDescription | |||
&& blob_id.hash == self.header.chain_id.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that only if self.header.height
is zero. It's sufficient if the first block requires the blob, and it might create less overhead managing blobs later. Also, then it matches required_blob_ids
again.
|
||
/* TODO: figure out if this can be fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(We could take one example chain description and assert its hash, just so we can't inadvertently change how chain IDs are computed. Not sure if that's useful.)
/// The epoch in which the chain is created. | ||
pub epoch: Epoch, | ||
/// Blob IDs of the committees corresponding to epochs. | ||
pub committees: BTreeMap<Epoch, Vec<u8>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this blob depends on other blobs, which complicates chain initialization, so we thought we'd put the full committees in here for now. We could change that as an optimization later.
pub struct ChainDescription { | ||
origin: ChainOrigin, | ||
timestamp: Timestamp, | ||
config: OpenChainConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider inlining this type here. (Probably in a later PR.)
@@ -445,11 +443,30 @@ where | |||
} | |||
|
|||
/// Invariant for the states of active chains. | |||
pub fn ensure_is_active(&self) -> Result<(), ChainError> { | |||
pub async fn ensure_is_active(&mut self, local_time: Timestamp) -> Result<(), ChainError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe due for a rename
|
||
/// Rearranges the messages in the bundle so that the first message is an `OpenChain` message. | ||
/// Returns whether the `OpenChain` message was found at all. | ||
pub fn put_openchain_at_front(bundles: &mut [IncomingBundle]) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
/// The epoch in which the chain is created. | ||
pub epoch: Epoch, | ||
/// Blob IDs of the committees corresponding to epochs. | ||
pub committees: BTreeMap<Epoch, Vec<u8>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is inaccurate then, right?
Motivation
This will allow for easier initialization of the network at genesis, among other things.
Proposal
ChainDescription
will become a blob, andOpenChain
messages will be removed. Callingopen_chain
will create the blob, and the chain will actually get initialized when a block is proposed for it.Test Plan
Should be covered by CI.
Release Plan
Links
ChainDescription
with blobs #2390